Conversation
- Implement trezor hardware support via USB and Bluetooth
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Bumps bitkit-core version to 0.1.44 - Adds SendTransactionSection.kt
# Conflicts: # app/src/main/java/to/bitkit/ui/ContentView.kt # app/src/main/java/to/bitkit/ui/settings/AdvancedSettingsScreen.kt # app/src/main/java/to/bitkit/ui/settings/AdvancedSettingsViewModel.kt
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
app/src/main/java/to/bitkit/ui/screens/trezor/SendTransactionSection.kt
Outdated
Show resolved
Hide resolved
| @Suppress("LongParameterList") | ||
| @Composable | ||
| private fun TrezorContent( | ||
| trezorState: TrezorState, |
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/ConnectedDeviceSection.kt
Outdated
Show resolved
Hide resolved
| endpoint.direction == UsbConstants.USB_DIR_IN -> { | ||
| readEndpoint = endpoint | ||
| } | ||
| endpoint.type == UsbConstants.USB_ENDPOINT_XFER_INT && |
There was a problem hiding this comment.
Bug: bulkTransfer() called on interrupt endpoints - all USB reads/writes will fail
findUsbEndpoints() exclusively selects endpoints with type USB_ENDPOINT_XFER_INT (interrupt), but both readUsbChunk and writeUsbChunk call UsbDeviceConnection.bulkTransfer() on those endpoints. Per Android's documented API contract, bulkTransfer() is only valid for bulk-type endpoints (USB_ENDPOINT_XFER_BULK); calling it with interrupt endpoints returns -1 on every invocation.
This means all USB communication with the Trezor will silently fail.
Fix: Either change findUsbEndpoints to filter for USB_ENDPOINT_XFER_BULK (if the Trezor exposes bulk endpoints on the active interface), or switch to UsbRequest with queue()/requestWait() for proper interrupt transfer handling.
|
|
||
| val disconnected = disconnectLatch.await(DISCONNECT_TIMEOUT_MS, TimeUnit.MILLISECONDS) | ||
| if (!disconnected) { | ||
| Logger.warn("BLE disconnect timeout, forcing close: '$path'", context = TAG) |
There was a problem hiding this comment.
Bug: userInitiatedCloseSet leaks path on disconnect timeout, silently suppressing future external disconnects
closeBleDevice() adds path to userInitiatedCloseSet at line ~829, relying on onConnectionStateChange to remove it. However, when disconnectLatch.await() times out (the if (!disconnected) branch here), gatt.close() is called immediately after - which can prevent the GATT callback from ever firing for this disconnect. The path then remains in userInitiatedCloseSet indefinitely.
On the next reconnect + external disconnect cycle for the same device path, onConnectionStateChange calls userInitiatedCloseSet.remove(path) which returns true (stale entry), suppressing _externalDisconnect.tryEmit(path). The app's state machine will permanently believe the device is connected.
Fix: Ensure userInitiatedCloseSet.remove(path) always runs after the disconnect attempt, regardless of timeout or exception, by adding it to a finally block after the latch await.
| "Scan found ${scannedDevices.size} devices: ${scannedDevices.map { it.id }}", | ||
| ) | ||
| val exactMatch = scannedDevices.find { it.id == deviceId } | ||
| val usbDevice = scannedDevices.find { it.transportType == TrezorTransportType.USB } |
There was a problem hiding this comment.
Security bug: connectKnownDevice can silently connect to a different Trezor device
When the known device is Bluetooth and a USB device is visible, the code substitutes the first USB device found in scan results with no identity verification:
val usbDevice = scannedDevices.find { it.transportType == TrezorTransportType.USB }This selects any USB device - not necessarily the same Trezor. In a multi-device environment or with a malicious USB accessory, this would connect to the wrong hardware wallet, sign Bitcoin transactions with the wrong seed, and overwrite the stored known device record with the substitute's ID.
Compare with autoReconnect() which correctly constrains the USB fallback to known device IDs (it.id in knownIds). The same guard should apply here, or cross-transport identity verification (e.g., matching device label/model after connecting) should be performed before trusting the substituted device.
| synchronized(pairingCodeLock) { | ||
| submittedPairingCode = "" | ||
| pairingCodeRequest = PairingCodeRequest(isRequested = true, latch = latch) | ||
| _needsPairingCode.value = true |
There was a problem hiding this comment.
CLAUDE.md violation: _stateFlow.value = used instead of _stateFlow.update {}
Per CLAUDE.md: "ALWAYS use _uiState.update { }, NEVER use _stateFlow.value ="
_needsPairingCode.value = is assigned directly in 4 places:
- Line 294:
_needsPairingCode.value = true - Line 303:
_needsPairingCode.value = false - Line 312:
_needsPairingCode.value = false - Line 349:
_needsPairingCode.value = false
All four should use .update { } instead, e.g.:
_needsPairingCode.update { true }
_needsPairingCode.update { false }| private suspend fun connectWithThpRetry(deviceId: String): TrezorFeatures { | ||
| TrezorDebugLog.log("THPRetry", "First connect attempt for: $deviceId") | ||
| logCredentialFileState(deviceId, "BEFORE 1st attempt") | ||
| return try { |
There was a problem hiding this comment.
CLAUDE.md violation: try-catch used instead of the Result API
Per CLAUDE.md: "ALWAYS use the Result API instead of try-catch" and "NEVER use Exception directly, use AppError instead"
connectWithThpRetry uses a raw try { } catch (e: Exception) { } block. The @Suppress("TooGenericExceptionCaught") annotation confirms the author was aware of the broad catch. This should be refactored to use runCatching { } with Result API chaining (onSuccess, onFailure, getOrElse).
Related: isRetryableError(e: Exception) at line ~623 also uses Exception as a parameter type directly, which should be AppError per CLAUDE.md.
| suspend fun autoReconnect(walletIndex: Int = 0): Result<TrezorFeatures> = withContext(bgDispatcher) { | ||
| val knownDevices = _state.value.knownDevices.ifEmpty { loadKnownDevices() } | ||
| if (knownDevices.isEmpty()) { | ||
| return@withContext Result.failure(IllegalStateException("No known devices")) |
There was a problem hiding this comment.
CLAUDE.md violation: IllegalStateException used instead of AppError
Per CLAUDE.md: "NEVER use Exception directly, use AppError instead" and "ALWAYS inherit custom exceptions from AppError"
IllegalStateException is a raw JVM exception, not an AppError subclass. Two occurrences:
- Line 381 (in
autoReconnect):Result.failure(IllegalStateException("No known devices")) - Line 413 (in
connectKnownDevice):Result.failure(IllegalStateException("Connection already in progress"))
Both should use AppError (or a custom subclass) instead.
app/src/main/java/to/bitkit/ui/screens/trezor/PairingCodeDialog.kt
Outdated
Show resolved
Hide resolved
- Upgrades to the latest bitkit-core bindings v0.1.45 - Updates the implementation accordingly
2 participants
Description
The intention of this Trezor dev dashboard is to serve as a place for developers to test and troubleshoot Trezor hardware features as they emerge. This is not meant to be user facing, but to serve as a reference for how to use and interact with Trezor hardware devices once work on user-facing Trezor features begin by native devs.
Settings->Advanced->TrezorPreview
QA Notes